-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(inputs.cgroup): Support more cgroup v2 formats #16474
base: master
Are you sure you want to change the base?
Conversation
Thanks so much for the pull request! |
f7f61cd
to
ee5c080
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @psnszsn! Just one concern regarding potential type conflicts in the code.
func numberOrString(s string) interface{} { | ||
i, err := strconv.ParseInt(s, 10, 64) | ||
if err == nil { | ||
return i | ||
} | ||
f, err := strconv.ParseFloat(s, 64) | ||
if err == nil { | ||
return f | ||
} | ||
|
||
return s | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, this concerns me a bit as this calls for type conflicts. Imagine you get
MY_KEY=4
in a first gather cycle but
MY_KEY=3.14
in a second cycle. The very same field will be an integer in the first cycle but a float in the second cycle! This cases all sorts of issues on the output side. We've been there in the past...
I'm not sure if the issue exists for cgroups as the problem could be avoided if floats always contain a decimal separator but if that's not guaranteed I suggest to either have a named list of fields being float or to add a flag to always force fields into float (and not int).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand the only files that have numbers with a fractional part are {cpu, io, memory}.pressure
that use the PSI format.
They always have the decimal separator so in that case they will always be parsed as floats:
$ cat /sys/fs/cgroup/cpu.pressure
some avg10=0.00 avg60=0.00 avg300=0.00 total=397576165
full avg10=0.00 avg60=0.00 avg300=0.00 total=376593821
However, the issue you are describing could also happen for values that have "max" as the default but are otherwise ints... In that case what would be a good alternative to the "max" string? Maybe math.MaxInt64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are fields with the max
string always of the integer type? If so, math.MaxInt64
would be the best way to go. For the PSI/float metrics, it seems like they always contain the decimal separator (e.g. 0.00
) so this should be good. Maybe add a comment to the code for the next one stumbling upon this?
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 🥳 This pull request decreases the Telegraf binary size by -1.62 % for linux amd64 (new size: 284.0 MB, nightly size 288.6 MB) 📦 Click here to get additional PR build artifactsArtifact URLs |
Summary
Add support for the following cgroup v2 formats:
cpu.pressure
,io.pressure
andmemory.pressure
KEY=VAL
used byhugetlb.*.numa_stat
Checklist
Related issues
resolves #16473